Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Emit Tree objects in topological order #16463

Closed

Conversation

EdSchouten
Copy link
Contributor

remote-apis PR 230 added a way where producers of Tree messages can indicate that the directories contained within are stored in topological order. The advantage of using such an ordering is that it permits instantiation of such objects onto a local file system in a streaming fashion. The same holds for lookups of individual paths.

Even though Bazel currently does not gain from this, this change at least modifies Bazel's REv2 client to emit topologically sorted trees. This makes it possible for tools such as Buildbarn's bb-browser to process them more efficiently.

More details:

@EdSchouten EdSchouten added the team-Remote-Exec Issues and PRs for the Execution (Remote) team label Oct 13, 2022
@EdSchouten EdSchouten force-pushed the eschouten/20221013-toposort branch from 9a34066 to 530c6a7 Compare October 23, 2022 02:14
@EdSchouten EdSchouten marked this pull request as ready for review October 23, 2022 02:15
@EdSchouten EdSchouten requested a review from a team as a code owner October 23, 2022 02:15
@EdSchouten EdSchouten requested review from coeuvre and tjgq October 23, 2022 02:15
Copy link
Member

@coeuvre coeuvre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@coeuvre coeuvre added the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Oct 24, 2022
@sgowroji
Copy link
Member

Hello @coeuvre, Above PR has ThirdParty changes, Could you please guide which code should be imported first. @EdSchouten Can you please squash above commits into one. Thanks!

@coeuvre
Copy link
Member

coeuvre commented Oct 26, 2022

We need to import third-party changes first because other changes in non third-party directories depend on them.

@EdSchouten
Copy link
Contributor Author

Hey there! So given @coeuvre's response, you don't want me to squash the commits?

@sgowroji
Copy link
Member

We still need all commits, squash into one.

remote-apis PR 230 added a way where producers of Tree messages can
indicate that the directories contained within are stored in topological
order. The advantage of using such an ordering is that it permits
instantiation of such objects onto a local file system in a streaming
fashion. The same holds for lookups of individual paths.

Even though Bazel currently does not gain from this, this change at
least modifies Bazel's REv2 client to emit topologically sorted trees.
This makes it possible for tools such as Buildbarn's bb-browser to
process them more efficiently.

More details:
- bazelbuild/remote-apis#229
- bazelbuild/remote-apis#230
@EdSchouten EdSchouten force-pushed the eschouten/20221013-toposort branch from 530c6a7 to e26e96a Compare October 27, 2022 15:37
@EdSchouten
Copy link
Contributor Author

Squashed!

copybara-service bot pushed a commit that referenced this pull request Oct 27, 2022
remote-apis PR 230 added a way where producers of Tree messages can
indicate that the directories contained within are stored in topological
order. The advantage of using such an ordering is that it permits
instantiation of such objects onto a local file system in a streaming
fashion. The same holds for lookups of individual paths.

Even though Bazel currently does not gain from this, this change at
least modifies Bazel's REv2 client to emit topologically sorted trees.
This makes it possible for tools such as Buildbarn's bb-browser to
process them more efficiently.

More details:
- bazelbuild/remote-apis#229
- bazelbuild/remote-apis#230

Partial commit for third_party/*, see #16463.

Signed-off-by: Sunil Gowroji <[email protected]>
@EdSchouten
Copy link
Contributor Author

Hey @sgowroji,
It seems the changes to the .proto files have already been merged now. Do I need to do anything on my end (e.g., rebase) for the second half of this change?

@sgowroji
Copy link
Member

sgowroji commented Nov 5, 2022

Remaining changes are already in progress through a internal CL. We will update you once it is merged. Thank you for the patience.

@copybara-service copybara-service bot closed this in c20d7ed Nov 9, 2022
@sgowroji sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Nov 9, 2022
@brentleyjones
Copy link
Contributor

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Nov 9, 2022
@EdSchouten EdSchouten deleted the eschouten/20221013-toposort branch November 11, 2022 04:55
@meteorcloudy
Copy link
Member

@bazel-io fork 6.0.0

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Dec 1, 2022
ShreeM01 pushed a commit that referenced this pull request Dec 1, 2022
remote-apis PR 230 added a way where producers of Tree messages can    indicate that the directories contained within are stored in topological    order. The advantage of using such an ordering is that it permits    instantiation of such objects onto a local file system in a streaming    fashion. The same holds for lookups of individual paths.

Even though Bazel currently does not gain from this, this change at    least modifies Bazel's REv2 client to emit topologically sorted trees.    This makes it possible for tools such as Buildbarn's bb-browser to    process them more efficiently.

More details:
- bazelbuild/remote-apis#229
- bazelbuild/remote-apis#230

Closes #16463.

PiperOrigin-RevId: 487196375
Change-Id: Iafcfd617fc101fec7bfa943552113ce57ab8041b
ShreeM01 pushed a commit that referenced this pull request Dec 1, 2022
remote-apis PR 230 added a way where producers of Tree messages can
indicate that the directories contained within are stored in topological
order. The advantage of using such an ordering is that it permits
instantiation of such objects onto a local file system in a streaming
fashion. The same holds for lookups of individual paths.

Even though Bazel currently does not gain from this, this change at
least modifies Bazel's REv2 client to emit topologically sorted trees.
This makes it possible for tools such as Buildbarn's bb-browser to
process them more efficiently.

More details:
- bazelbuild/remote-apis#229
- bazelbuild/remote-apis#230

Partial commit for third_party/*, see #16463.

Signed-off-by: Sunil Gowroji <[email protected]>
meteorcloudy pushed a commit that referenced this pull request Dec 2, 2022
* Emit Tree objects in topological order

remote-apis PR 230 added a way where producers of Tree messages can    indicate that the directories contained within are stored in topological    order. The advantage of using such an ordering is that it permits    instantiation of such objects onto a local file system in a streaming    fashion. The same holds for lookups of individual paths.

Even though Bazel currently does not gain from this, this change at    least modifies Bazel's REv2 client to emit topologically sorted trees.    This makes it possible for tools such as Buildbarn's bb-browser to    process them more efficiently.

More details:
- bazelbuild/remote-apis#229
- bazelbuild/remote-apis#230

Closes #16463.

PiperOrigin-RevId: 487196375
Change-Id: Iafcfd617fc101fec7bfa943552113ce57ab8041b

* Emit Tree objects in topological order

remote-apis PR 230 added a way where producers of Tree messages can
indicate that the directories contained within are stored in topological
order. The advantage of using such an ordering is that it permits
instantiation of such objects onto a local file system in a streaming
fashion. The same holds for lookups of individual paths.

Even though Bazel currently does not gain from this, this change at
least modifies Bazel's REv2 client to emit topologically sorted trees.
This makes it possible for tools such as Buildbarn's bb-browser to
process them more efficiently.

More details:
- bazelbuild/remote-apis#229
- bazelbuild/remote-apis#230

Partial commit for third_party/*, see #16463.

Signed-off-by: Sunil Gowroji <[email protected]>

Signed-off-by: Sunil Gowroji <[email protected]>
Co-authored-by: Ed Schouten <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Remote-Exec Issues and PRs for the Execution (Remote) team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants